Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Applying John Papa Style Guide to generator-ng-component #17

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Applying John Papa Style Guide to generator-ng-component #17

wants to merge 7 commits into from

Conversation

remicastaing
Copy link
Contributor

Here is an attempt to comply to John Papa Style Guide as evoked in angular-fullstack/generator-angular-fullstack#1119.

- apply JPSG to controlle.jsr
- remove $scope an replace by this
- apply JPSG to controller.specs.js
- apply JPSG to route
- use"controller as"
@kingcody
Copy link
Collaborator

kingcody commented Aug 5, 2015

Hi @remicastaing, thanks for the work you've done here. A couple of thoughts...

If we use https://github.com/johnpapa/angular-styleguide#style-y024, then we'll need to:

FN.$inject = ['injectable1', 'injectable2', 'injectable3'];

for every (controller|service|factory|provider)... So I am personally not 100% sold on that.

Also, if we don't end up using named functions, then I don't really see a need in the IIFE's since we won't be polluting the global namespace.

Other than that, I'm good with everything else (:

@remicastaing
Copy link
Contributor Author

Hello @kingcody. I didn't complied to rule y024, because it worked for me without. We should instead comply to https://github.com/johnpapa/angular-styleguide#style-y100 (comment functions that need automated dependency injection using /** @ngInject */).

I like to name functions because it's, IMO, more readable and the file is flatter, so I added IIFE to all files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants